Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle vc reboot session invalid for listview #3064

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Handle vc reboot session invalid for listview #3064

merged 1 commit into from
Oct 16, 2024

Conversation

adikul30
Copy link
Contributor

@adikul30 adikul30 commented Sep 24, 2024 •
edited
Loading

What this PR does / why we need it:

This PR refactors the listview session logic.

A new isReady boolean represents the status of the listview + property collector mechanism. When this is true, the property collector is listening, when this is false, it's in the process of being created/re-created.

ResetVirtualCenter will store the latest VirtualCenter object reference when a credential change happens but not invalidate the ongoing property collector connection.

isSessionValid will check if the ongoing session is valid. If it's invalid, it will set the isReady to false and cancel the context on which the property collector is running. This will cause the listview object and property collector to be re-created but this time with the most updated credentials.

AddTask & RemoveTask will first check if the isReady state before performing any other operations. Even after checking if isReady is true and session is valid but adding task to listview returns an error, this means that the credentials with which the listview object was created is no longer valid. So, it will set the isReady to false, cancel the property collector context, return failure and let listenToTaskUpdates() re-create the listview object and start listening again.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Testing done:

session killed from VC

{"level":"error","time":"2024-10-11T17:53:39.853251518Z","caller":"volume/listview.go:334","msg":"WaitForUpdates returned err: destroy property filter failed with ServerFaultCode: The session is not authenticated. after failing to wait for updates: ServerFaultCode: The task was canceled by a user. for vc: sc2-10-212-76-196.nimbus.eng.vmware.com","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a","stacktrace":"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume.(*ListViewImpl).listenToTaskUpdates\n\t/build/pkg/common/cns-lib/volume/listview.go:334"}
{"level":"info","time":"2024-10-11T17:53:39.855328528Z","caller":"volume/listview.go:338","msg":"waiting for lock","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}
{"level":"info","time":"2024-10-11T17:53:39.855382396Z","caller":"volume/listview.go:340","msg":"setting listview ready state to false. current ready state: true","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}
{"level":"info","time":"2024-10-11T17:53:39.855669607Z","caller":"volume/listview.go:342","msg":"listview ready state is false","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}
{"level":"info","time":"2024-10-11T17:53:39.873438166Z","caller":"vsphere/virtualcenter.go:330","msg":"logging out current session and clearing idle sessions","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}
{"level":"error","time":"2024-10-11T17:53:39.894861336Z","caller":"vsphere/virtualcenter.go:335","msg":"failed to logout current session. still clearing idle sessions. err: ServerFaultCode: The session is not authenticated.","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a","stacktrace":"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/vsphere.(*VirtualCenter).connect\n\t/build/pkg/common/cns-lib/vsphere/virtualcenter.go:335\nsigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/vsphere.(*VirtualCenter).Connect\n\t/build/pkg/common/cns-lib/vsphere/virtualcenter.go:270\nsigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume.(*ListViewImpl).isConnected\n\t/build/pkg/common/cns-lib/volume/listview.go:269\nsigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume.(*ListViewImpl).listenToTaskUpdates\n\t/build/pkg/common/cns-lib/volume/listview.go:286"}
{"level":"info","time":"2024-10-11T17:53:39.897345568Z","caller":"vsphere/virtualcenter.go:340","msg":"Creating a new client session as the existing one isn't valid or not authenticated","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}
{"level":"info","time":"2024-10-11T17:53:40.076817656Z","caller":"vsphere/virtualcenter.go:213","msg":"New session ID for 'VSPHERE.LOCAL\\wcp-storage-user-66a68fad-7ddc-4980-a47d-8c44b0ae9faa-e37e61cd-fdc0-4f3a-b9c9-359b7c79c24b' = 52d9b959-3589-bcb3-1f8e-36a40c861986","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}
{"level":"info","time":"2024-10-11T17:53:40.084088176Z","caller":"volume/listview.go:291","msg":"connection to vc successful","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}
{"level":"info","time":"2024-10-11T17:53:40.084445873Z","caller":"volume/listview.go:294","msg":"attempting lock before ","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}
{"level":"info","time":"2024-10-11T17:53:40.084479217Z","caller":"volume/listview.go:297","msg":"re-creating the listView object","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}
{"level":"info","time":"2024-10-11T17:53:40.11002682Z","caller":"volume/listview.go:103","msg":"created listView object ListView:session[52f2780b-478c-55b1-2bc5-18f2aed7521b]5296bda8-d591-a205-2d8d-b7133e0abd31 for virtualCenter: sc2-10-212-76-196.nimbus.eng.vmware.com","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}
{"level":"info","time":"2024-10-11T17:53:40.111001425Z","caller":"volume/listview.go:310","msg":"Starting listening for task updates...","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}
{"level":"info","time":"2024-10-11T17:53:40.111061607Z","caller":"volume/listview.go:311","msg":"waitForUpdatesContext context.Background.WithCancel","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}
{"level":"info","time":"2024-10-11T17:53:40.11107162Z","caller":"volume/listview.go:314","msg":"listview ready state is true","TraceId":"083664d8-85fe-4a1e-acc3-5e2c183f2c3a"}

2/2 passing for snapshot restore during VC reboot test but logs not captured and setup is now cleaned up.

Special notes for your reviewer:

Release note:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 24, 2024
@adikul30 adikul30 changed the title Handle vc reboot session invalid for listview (wip) Handle vc reboot session invalid for listview Sep 24, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2024
@adikul30 adikul30 force-pushed the topic/adkulkarni/vc-reboot-session-fix branch 2 times, most recently from 6b476b1 to 16f8332 Compare September 26, 2024 21:15
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 26, 2024
@adikul30 adikul30 force-pushed the topic/adkulkarni/vc-reboot-session-fix branch from 16f8332 to d6e7490 Compare September 26, 2024 21:38
@adikul30 adikul30 force-pushed the topic/adkulkarni/vc-reboot-session-fix branch 2 times, most recently from e514174 to f87dde9 Compare October 10, 2024 20:47
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 10, 2024
@adikul30 adikul30 force-pushed the topic/adkulkarni/vc-reboot-session-fix branch from f87dde9 to 8938cff Compare October 10, 2024 20:51
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2024
@adikul30 adikul30 force-pushed the topic/adkulkarni/vc-reboot-session-fix branch 3 times, most recently from ae97b15 to 6addbcf Compare October 10, 2024 21:47
@deepakkinni
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 11, 2024
@deepakkinni
Copy link
Contributor

/approve

@adikul30 adikul30 changed the title (wip) Handle vc reboot session invalid for listview Handle vc reboot session invalid for listview Oct 14, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2024
@adikul30 adikul30 force-pushed the topic/adkulkarni/vc-reboot-session-fix branch from 6addbcf to 9fa791a Compare October 16, 2024 18:49
divyenpatel
divyenpatel approved these changes Oct 16, 2024
View reviewed changes
Copy link
Member

@divyenpatel divyenpatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adikul30, deepakkinni, divyenpatel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [adikul30,deepakkinni,divyenpatel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit e05fdae into kubernetes-sigs:master Oct 16, 2024
12 checks passed
@adikul30 adikul30 mentioned this pull request Oct 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divyenpatel divyenpatel divyenpatel approved these changes

@chethanv28 chethanv28 Awaiting requested review from chethanv28

@xing-yang xing-yang Awaiting requested review from xing-yang

Assignees

@divyenpatel divyenpatel

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adikul30 @deepakkinni @k8s-ci-robot @divyenpatel